-
Notifications
You must be signed in to change notification settings - Fork 65
feat: mutiple backends for recipes #3114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Fixes containers#3006 Signed-off-by: Jeff MAURY <[email protected]>
Fixes containers#3006 Signed-off-by: Jeff MAURY <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried 2 recipes (with backend
and backends
):
backends
: all works as expected ✔️
When selecting recipe with just "backed"
Screencast_20250603_080529.webm
I'm unable to see any models (guessing that backend should be backends)
basedir: 'recipes/natural_language_processing/chatbot', | ||
readme: '', | ||
recommended: ['hf.instructlab.granite-7b-lab-GGUF', 'hf.instructlab.merlinite-7b-lab-GGUF'], | ||
backend: 'llama-cpp', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldnt this be also backends: ['llama-cpp'] ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not mandatory because it will be parsed but updated for consistency
packages/backend/src/assets/ai.json
Outdated
@@ -166,7 +166,7 @@ | |||
"hf.instructlab.merlinite-7b-lab-GGUF", | |||
"hf.lmstudio-community.granite-3.0-8b-instruct-GGUF" | |||
], | |||
"backend": "llama-cpp", | |||
"backend": ["llama-cpp", "openvino"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldnt there be also backends
like if I have an array -> backedns
Signed-off-by: Jeff MAURY <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
.flatMap(r => r.backends) | ||
.filter(b => b !== undefined) | ||
.filter((value, index, array) => array.indexOf(value) === index) | ||
.sort((a, b) => a.localeCompare(b)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remark (non-blocking): non related to this PR, but this filtering chain is unreadable, what is r
? what is b
?, what is the type of value
, what is the type of a
and b
? :/
let res: Recipe[] = []; | ||
for (const value of values) { | ||
res = [...res, ...result.filter(r => r.backends?.includes(value))]; | ||
} | ||
result = res; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: I don't understand what this code is doing? can we add a comment? filtering result? shallow copy?
@@ -42,7 +42,7 @@ let startedContainerProviderConnectionInfo: ContainerProviderConnectionInfo[] = | |||
let localPath: LocalRepository | undefined = $derived(findLocalRepositoryByRecipeId($localRepositories, recipe?.id)); | |||
// Filter all models based on backend property | |||
let models: ModelInfo[] = $derived( | |||
$modelsInfo.filter(model => (model.backend ?? InferenceType.NONE) === (recipe?.backend ?? InferenceType.NONE)), | |||
$modelsInfo.filter(model => recipe?.backends?.includes(model.backend ?? InferenceType.NONE)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I would be more confortable adding an unit test in StartRecipe.spec.ts
where we have a recipe with several backends, to ensure we properly display all models matching the backends listed.
@@ -42,7 +42,7 @@ let startedContainerProviderConnectionInfo: ContainerProviderConnectionInfo[] = | |||
let localPath: LocalRepository | undefined = $derived(findLocalRepositoryByRecipeId($localRepositories, recipe?.id)); | |||
// Filter all models based on backend property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comment, currently trying it locally on Windows 11, good job otherwise 👍
Signed-off-by: Jeff MAURY <[email protected]>
Fixes #3006
Signed-off-by: Jeff MAURY [email protected]
What does this PR do?
Allow association from recipe to multiple backends
Screenshot / video of UI
N/A
What issues does this PR fix or reference?
#3006
How to test this PR?
Start recipes. You should be a able to start recipes with an OpenVINO model